Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [M3-8960] - Update Kubernetes version upgrade components for LKE-E #11415

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Dec 12, 2024

Description 📝

With the addition of tiered versions, components that use the current useKubernetesVersionQuery to fetch all (standard)/versions need to be updated to use the new useKubernetesTieredVersionsQuery, which supports k8 versions for both existing standard and new enterprise tiers. This change will prevent the UI from recommending incorrect and incompatible version upgrades to clusters.

Changes 🔄

  • Adds a new hook useLkeStandardOrEnterpriseVersions to return the correct version data from queries and reduce duplicate logic until we're in GA
    • Uses hooks in version upgrade components and on the cluster create flow for version selection
  • Adds test coverage
    • For kubeUtil functions related to versions
    • For lke-upgrade.spec.ts to confirm enterprise versions can be upgraded

Preview 📷

Before After
Screenshot 2024-12-13 at 9 11 09 AM Screenshot 2024-12-13 at 9 10 31 AM
Screenshot 2024-12-13 at 9 11 26 AM Screenshot 2024-12-13 at 9 11 44 AM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have the customer tag for LKE-E on your account (see project tracker)
  • Create an LKE-E cluster

Reproduction steps

(How to reproduce the issue, if applicable)

  • On the develop branch, navigate to the Kubernetes landing page and observe that there is an 'Upgrade' chip on your LKE-E cluster, and clicking it recommends an incorrect upgrade version
  • Visit the LKE-E cluster's details page observe that the upgrade banner is displayed, recommending an incorrect upgrade version
  • Click on the Upgrade button in the banner and observe that an incorrect upgrade version is displayed

Verification steps

(How to verify changes)

  • On this branch, navigate to the Kubernetes landing page and observe that there is not an Upgrade chip for the LKE-E cluster because it's on the latest enterprise version
  • Visit the LKE-E cluster's details page observe that no upgrade banner is displayed because it's on the latest enterprise version
  • Confirm test coverage looks good and related tests pass:
yarn test kubeUtils
yarn cy:run -s "cypress/e2e/core/kubernetes/lke-create.spec.ts,cypress/e2e/core/kubernetes/lke-update.spec.ts"
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

Sorry, something went wrong.

@mjac0bs mjac0bs force-pushed the M3-8960-update-kube-version-upgrade-components-for-lke-e branch from 6db07ca to b2ab4fb Compare December 13, 2024 15:06
@mjac0bs mjac0bs marked this pull request as ready for review December 13, 2024 17:01
@mjac0bs mjac0bs requested review from a team as code owners December 13, 2024 17:01
@mjac0bs mjac0bs requested review from jdamore-linode, carrillo-erik and harsh-akamai and removed request for a team December 13, 2024 17:01
@@ -303,7 +287,7 @@ export const CreateCluster = () => {
selectedRegionID: selectedRegionId,
});

if (typesError || regionsError || versionLoadError) {
if (typesError || regionsError || versionsError) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates with error and loading variables were just to update to the values returned from the hook.

Now the tiered version query is disabled when the LKE-E feature is disabled (and isFetching will be false), so we shouldn't need to check isLkeEnterpriseLAFeatureEnabled && enterpriseTierVersionDataIsLoading.

const { data: pools } = useAllKubernetesNodePoolQuery(cluster.id);
const typesQuery = useSpecificTypes(pools?.map((pool) => pool.type) ?? []);
const types = extendTypesQueryResult(typesQuery);
const { data: regions } = useRegionsQuery();

const region = regions?.find((r) => r.id === cluster.region);

const { versions } = useLkeStandardOrEnterpriseVersions(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We default to standard if the tier is undefined (LKE-E feature not enabled) because all current clusters are 'standard'.

* If LKE-E is disabled, use the data from the existing /versions endpoint.
* @todo LKE-E: Clean up use of versionData once LKE-E is in GA.
*/
const versionData =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was moved inside useLkeStandardOrEnterpriseVersions.

it('should return default fallback value when called with empty versions', () => {
const result = getLatestVersion([]);
expect(result).toEqual({ label: '', value: '' });
});
});

describe('getNextVersion', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test coverage for this previously untested function since the upgrade dialog uses it.

capabilities: [],
},
describe('hooks', () => {
describe('useIsLkeEnterpriseEnabled', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 241-308 (tests for useIsLkeEnterpriseEnabled) are not new, I just wrapped these tests in a describe('hooks'... so they're in the diff.

What is new are the tests for useLkeStandardOrEnterpriseVersions.

Copy link

github-actions bot commented Dec 13, 2024

Coverage Report:
Base Coverage: 86.98%
Current Coverage: 86.98%

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality is looking good!

Comment on lines +304 to +305
const oldVersion = '1.31.1+lke1';
const newVersion = '1.31.1+lke2';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are new LKE-E versions denoted by a higher number after lke or the "base" standard version number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both? Versioning is what confuses me the most right now, tbh.

The LKE folks said earier:

LKE-E exposes the patch version, and an overall "lke" version, so the customer is able to specifically choose when to include any upgrade of any component. in both LKE and LKE-E only the "latest" version of each kubernetes minor version is available. In other words, if 1.31.1+lke1 is installed, and then 1.31.2+lke1 comes out. New installs can only choose 1.31.2+lke1, not any arbitrary version

So there are still minor version increases, and patch increases, and then there's this new(ly exposed?) "lke" version and that can change too.

And from some LKE-E docs:

Only one patch version per minor version will be supported at any point in time. For example, at a point in time, the following would be the supported versions “v1.29.8-lke1”, “v1.30.2-lke2”, “v1.31.1-lke1”. Customers will be notified and prompted in Cloud Manager when there is a new patch version available. For example, a customer who created a cluster with ““v1.30.1-lke1” would be notified when ““v1.30.1-lke2” or “v1.30.2-lke2”” is available.

I think that last part is consistent with the test spec here. We have a patch version that gets a new lke version, so that is considered the latest patch to update to.

packages/manager/src/queries/kubernetes.ts Show resolved Hide resolved
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @mjac0bs! Thanks for the tests!

Comment on lines 82 to 94
export const kubernetesStandardTierVersionFactory = Factory.Sync.makeFactory<KubernetesTieredVersion>(
{
id: '1.31',
tier: 'standard',
}
);

export const kubernetesEnterpriseTierVersionFactory = Factory.Sync.makeFactory<KubernetesTieredVersion>(
{
id: 'v1.31.1+lke1',
tier: 'enterprise',
}
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make any sense to use Factory.each((i) => ... on these IDs to let the values increment, or would it just complicate things / be inconsistent with the actual data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0f35afa. 👍🏼 I incremented the minor version in the standard tier factory, and the patch in the enterprise tier factory. Tests continue to pass and I think this will be consistent enough with possible data/not overly complicated.

@@ -133,7 +134,7 @@ describe('LKE cluster updates', () => {
* - Confirms that Kubernetes upgrade prompt is shown when not up-to-date.
* - Confirms that Kubernetes upgrade prompt is hidden when up-to-date.
*/
it('can upgrade kubernetes version from the details page', () => {
it('can upgrade standard kubernetes version from the details page', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this distinction! (+ the other test)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts about unit tests to confirm that the Upgrade chip is present when a new LKE version is available and is absent when there is no new version? Don't think it needs to block this PR but might be good to have as a small standalone test ticket.

Copy link
Contributor Author

@mjac0bs mjac0bs Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call out - yeah, let's do this in a standalone test ticket. It looks like the LKE landing page spec isn't testing version upgrades chip at all currently. I made M3-9023 and will get that done as part of this LKE-E epic!

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Dec 16, 2024
Copy link
Contributor

@harsh-akamai harsh-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good! Thanks @mjac0bs

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 468 passing tests on test run #7 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing468 Passing2 Skipped89m 8s

@mjac0bs mjac0bs merged commit d5fb831 into linode:develop Dec 17, 2024
23 checks passed
Copy link

cypress bot commented Dec 17, 2024

Cloud Manager E2E    Run #6982

Run Properties:  status check failed Failed #6982  •  git commit d5fb831f1b: upcoming: [M3-8960] - Update Kubernetes version upgrade components for LKE-E (#1...
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6982
Run duration 30m 13s
Commit git commit d5fb831f1b: upcoming: [M3-8960] - Update Kubernetes version upgrade components for LKE-E (#1...
Committer Mariah Jacobs
View all properties for this run ↗︎

Test results
Tests that failed  Failures 3
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 2
Tests that passed  Passing 463
View all changes introduced in this branch ↗︎

Tests for review

Failed  linodes/rebuild-linode.spec.ts • 2 failed tests

View Output Video

Test Artifacts
rebuild linode > rebuilds a linode from Community StackScript Screenshots Video
rebuild linode > rebuilds a linode from Account StackScript Screenshots Video
Failed  objectStorage/object-storage.e2e.spec.ts • 1 failed test

View Output Video

Test Artifacts
object storage end-to-end tests > can create and delete object storage buckets Screenshots Video

dmcintyr-akamai pushed a commit to dmcintyr-akamai/manager that referenced this pull request Jan 9, 2025
…r LKE-E (linode#11415)

* Make a hook because this is getting repetitive

* Determine enterprise or standard versions with hook

* Update versions type for util - need to test

* Improve hook and use in cluster create flow

* Move hook to kubeUtils

* Add test coverage and factories

* Clean up kubeUtils.test.ts

* Add kubeUtils helper function test coverage for LKE-E

* Update Cypress test coverage for enterprise version upgrades

* Clean up

* Added changeset: Update Kubernetes version upgrade components for LKE-E

* Fix test failure by mocking the LKE-E capability needed

* Update TODO

* Address PR feedback: increment ids in tiered version factories
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! LKE-Enterprise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants