-
Notifications
You must be signed in to change notification settings - Fork 702
CONSOLE-5296: Add non-scalable image warning when scaling workloads #16436
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
Open
swshende-cmd
wants to merge
6
commits into
openshift:main
Choose a base branch
from
swshende-cmd:RFE-3935
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f5a5079
RFE-3935: Add non-scalable image warning when scaling workloads
shendeswapnil6 2075e36
fixup: Address CodeRabbit review feedback
shendeswapnil6 fc269b0
fixup: Run yarn i18n to fix locale key placement
shendeswapnil6 98268ce
fixup: Fix i18n key placement and trailing newline
shendeswapnil6 519d578
Address PR review feedback: use useK8sWatchResource, deduplicate JSX
shendeswapnil6 3e01915
fixup: Add ImageStreamTagResource type, stabilize dcTriggers dep
shendeswapnil6 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
155 changes: 155 additions & 0 deletions
155
frontend/packages/console-shared/src/hooks/__tests__/useNonScalableImageCheck.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| import { renderHook } from '@testing-library/react'; | ||
| import { useK8sWatchResource } from '@console/internal/components/utils/k8s-watch-hook'; | ||
| import { useNonScalableImageCheck } from '../useNonScalableImageCheck'; | ||
|
|
||
| jest.mock('@console/internal/components/utils/k8s-watch-hook', () => ({ | ||
| useK8sWatchResource: jest.fn(), | ||
| })); | ||
|
|
||
| const mockUseK8sWatchResource = useK8sWatchResource as jest.Mock; | ||
|
|
||
| const makeDeploymentConfig = (istName?: string, namespace = 'test-ns') => ({ | ||
| kind: 'DeploymentConfig', | ||
| apiVersion: 'apps.openshift.io/v1', | ||
| metadata: { name: 'test-dc', namespace }, | ||
| spec: { | ||
| replicas: 1, | ||
| triggers: istName | ||
| ? [ | ||
| { | ||
| type: 'ImageChange', | ||
| imageChangeParams: { | ||
| from: { kind: 'ImageStreamTag', name: istName, namespace }, | ||
| }, | ||
| }, | ||
| ] | ||
| : [], | ||
| }, | ||
| }); | ||
|
|
||
| const makeDeployment = (istName?: string, namespace = 'test-ns') => ({ | ||
| kind: 'Deployment', | ||
| apiVersion: 'apps/v1', | ||
| metadata: { | ||
| name: 'test-deploy', | ||
| namespace, | ||
| annotations: istName | ||
| ? { | ||
| 'image.openshift.io/triggers': JSON.stringify([ | ||
| { from: { kind: 'ImageStreamTag', name: istName, namespace } }, | ||
| ]), | ||
| } | ||
| : {}, | ||
| }, | ||
| spec: { replicas: 1 }, | ||
| }); | ||
|
|
||
| const makeIST = (nonScalable?: string) => ({ | ||
| image: { | ||
| dockerImageMetadata: { | ||
| Config: { | ||
| Labels: nonScalable !== undefined ? { 'io.openshift.non-scalable': nonScalable } : {}, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| describe('useNonScalableImageCheck', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should return isNonScalable=true when IST has io.openshift.non-scalable=true', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([makeIST('true'), true, null]); | ||
| const resource = makeDeploymentConfig('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(true); | ||
| expect(result.current.loading).toBe(false); | ||
| }); | ||
|
|
||
| it('should return isNonScalable=false when IST does not have the label', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([makeIST(), true, null]); | ||
| const resource = makeDeploymentConfig('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(result.current.loading).toBe(false); | ||
| }); | ||
|
|
||
| it('should pass null to useK8sWatchResource when there are no triggers', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([null, true, null]); | ||
| const resource = makeDeploymentConfig(); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(mockUseK8sWatchResource).toHaveBeenCalledWith(null); | ||
| }); | ||
|
|
||
| it('should handle Deployment with image.openshift.io/triggers annotation', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([makeIST('true'), true, null]); | ||
| const resource = makeDeployment('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(true); | ||
| expect(mockUseK8sWatchResource).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| kind: 'ImageStreamTag', | ||
| name: 'myapp:latest', | ||
| namespace: 'test-ns', | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('should return isNonScalable=false when useK8sWatchResource returns an error', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([null, true, new Error('Forbidden')]); | ||
| const resource = makeDeploymentConfig('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(result.current.loading).toBe(false); | ||
| }); | ||
|
|
||
| it('should pass null to useK8sWatchResource for Deployment without trigger annotation', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([null, true, null]); | ||
| const resource = makeDeployment(); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(mockUseK8sWatchResource).toHaveBeenCalledWith(null); | ||
| }); | ||
|
|
||
| it('should return loading=true while useK8sWatchResource has not loaded', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([null, false, null]); | ||
| const resource = makeDeploymentConfig('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(result.current.loading).toBe(true); | ||
| }); | ||
|
|
||
| it('should pass null to useK8sWatchResource when resource is null', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([null, true, null]); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(null)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| expect(mockUseK8sWatchResource).toHaveBeenCalledWith(null); | ||
| }); | ||
|
|
||
| it('should return isNonScalable=false when label value is not "true"', () => { | ||
| mockUseK8sWatchResource.mockReturnValue([makeIST('false'), true, null]); | ||
| const resource = makeDeploymentConfig('myapp:latest'); | ||
|
|
||
| const { result } = renderHook(() => useNonScalableImageCheck(resource)); | ||
|
|
||
| expect(result.current.isNonScalable).toBe(false); | ||
| }); | ||
| }); |
119 changes: 119 additions & 0 deletions
119
frontend/packages/console-shared/src/hooks/useNonScalableImageCheck.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import { useMemo } from 'react'; | ||
| import { useK8sWatchResource } from '@console/internal/components/utils/k8s-watch-hook'; | ||
| import { ImageStreamTagModel } from '@console/internal/models'; | ||
| import type { K8sResourceKind } from '@console/internal/module/k8s'; | ||
|
|
||
| const NON_SCALABLE_LABEL = 'io.openshift.non-scalable'; | ||
| const IMAGE_TRIGGER_ANNOTATION = 'image.openshift.io/triggers'; | ||
|
|
||
| type ISTReference = { | ||
| name: string; | ||
| namespace: string; | ||
| }; | ||
|
|
||
| type ImageStreamTagResource = K8sResourceKind & { | ||
| image?: { | ||
| dockerImageMetadata?: { | ||
| Config?: { | ||
| Labels?: Record<string, string>; | ||
| }; | ||
| }; | ||
| }; | ||
| }; | ||
|
|
||
| const getISTFromDeploymentConfig = (resource: K8sResourceKind): ISTReference | null => { | ||
| const triggers = resource?.spec?.triggers; | ||
| if (!Array.isArray(triggers)) { | ||
| return null; | ||
| } | ||
| const imageChangeTrigger = triggers.find( | ||
| (trigger) => | ||
| trigger.type === 'ImageChange' && | ||
| trigger?.imageChangeParams?.from?.kind === 'ImageStreamTag' && | ||
| !!trigger?.imageChangeParams?.from?.name, | ||
| ); | ||
| if (!imageChangeTrigger) { | ||
| return null; | ||
| } | ||
| const { name, namespace } = imageChangeTrigger.imageChangeParams.from; | ||
| return { | ||
| name, | ||
| namespace: namespace || resource.metadata?.namespace, | ||
| }; | ||
| }; | ||
|
|
||
| const getISTFromTriggerAnnotation = (resource: K8sResourceKind): ISTReference | null => { | ||
| const annotation = resource?.metadata?.annotations?.[IMAGE_TRIGGER_ANNOTATION]; | ||
| if (!annotation) { | ||
| return null; | ||
| } | ||
| try { | ||
| const triggers = JSON.parse(annotation); | ||
| const trigger = Array.isArray(triggers) | ||
| ? triggers.find((t) => t?.from?.kind === 'ImageStreamTag' && !!t?.from?.name) | ||
| : null; | ||
| if (!trigger) { | ||
| return null; | ||
| } | ||
| return { | ||
| name: trigger.from.name, | ||
| namespace: trigger.from.namespace || resource.metadata?.namespace, | ||
| }; | ||
| } catch { | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| const getISTReference = (resource: K8sResourceKind): ISTReference | null => { | ||
| if (resource?.kind === 'DeploymentConfig') { | ||
| return getISTFromDeploymentConfig(resource); | ||
| } | ||
| return getISTFromTriggerAnnotation(resource); | ||
| }; | ||
|
|
||
| /** | ||
| * Checks if a workload's container image has the `io.openshift.non-scalable` label. | ||
| * Resolves the ImageStreamTag reference from the workload's triggers or annotations, | ||
| * watches the IST via `useK8sWatchResource`, and inspects | ||
| * `image.dockerImageMetadata.Config.Labels`. | ||
| * | ||
| * Pass `null` to skip the watch (e.g. for non-replica paths). | ||
| * Returns `{ isNonScalable: false }` silently on any error (missing IST, permissions, etc.). | ||
| */ | ||
| export const useNonScalableImageCheck = ( | ||
| resource: K8sResourceKind | null, | ||
| ): { isNonScalable: boolean; loading: boolean } => { | ||
| const triggerAnnotation = resource?.metadata?.annotations?.[IMAGE_TRIGGER_ANNOTATION]; | ||
| const resourceKind = resource?.kind; | ||
| const resourceNamespace = resource?.metadata?.namespace; | ||
| const dcISTName = resource?.spec?.triggers?.find( | ||
| (t) => t?.type === 'ImageChange' && t?.imageChangeParams?.from?.kind === 'ImageStreamTag', | ||
| )?.imageChangeParams?.from?.name; | ||
|
|
||
| const istRef = useMemo( | ||
| () => (resource ? getISTReference(resource) : null), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [resourceKind, resourceNamespace, triggerAnnotation, dcISTName], | ||
| ); | ||
|
|
||
| const istName = istRef?.name; | ||
| const istNamespace = istRef?.namespace; | ||
|
|
||
| const [ist, loaded, error] = useK8sWatchResource<ImageStreamTagResource>( | ||
| istName && istNamespace | ||
| ? { | ||
| kind: ImageStreamTagModel.kind, | ||
| name: istName, | ||
| namespace: istNamespace, | ||
| isList: false, | ||
| } | ||
| : null, | ||
| ); | ||
|
|
||
| const isNonScalable = | ||
| loaded && !error | ||
| ? ist?.image?.dockerImageMetadata?.Config?.Labels?.[NON_SCALABLE_LABEL] === 'true' | ||
| : false; | ||
|
|
||
| return { isNonScalable, loading: !loaded }; | ||
| }; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadingstate is ignored — user can scale before the check completesThe hook returns
{ isNonScalable, loading }but both consumers (PodRingandConfigureCountModal) destructure onlyisNonScalable. During the initial fetch,isNonScalabledefaults tofalse, so the warning won't show until the IST fetch resolves. If the check matters enough to exist, consider disabling the scale-up button or showing a spinner whileloadingistrue, to avoid a brief window where the user can scale without seeing the warning.