-
Notifications
You must be signed in to change notification settings - Fork 705
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
Changes from all commits
f5a5079
2075e36
fc269b0
98268ce
519d578
3e01915
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,13 @@ | ||
| import type { FC } from 'react'; | ||
| import { useState, useEffect } from 'react'; | ||
| import { Button, Split, SplitItem, Bullseye } from '@patternfly/react-core'; | ||
| import { Button, Split, SplitItem, Bullseye, Tooltip } from '@patternfly/react-core'; | ||
| import { AngleDownIcon, AngleUpIcon, AutomationIcon } from '@patternfly/react-icons'; | ||
| import * as _ from 'lodash'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import type { ImpersonateKind } from '@console/dynamic-plugin-sdk'; | ||
| import type { K8sResourceKind, K8sKind } from '@console/internal/module/k8s'; | ||
| import { k8sPatch } from '@console/internal/module/k8s'; | ||
| import { useNonScalableImageCheck } from '../../hooks/useNonScalableImageCheck'; | ||
| import { useRelatedHPA } from '../../hooks/useRelatedHPA'; | ||
| import type { ExtPodKind } from '../../types/pod'; | ||
| import { usePodRingLabel, usePodScalingAccessStatus } from '../../utils/pod-ring-utils'; | ||
|
|
@@ -82,6 +83,7 @@ export const PodRing: FC<PodRingProps> = ({ | |
| } = obj; | ||
| const [hpa] = useRelatedHPA(apiVersion, kind, name, namespace); | ||
| const hpaControlledScaling = !!hpa; | ||
| const { isNonScalable } = useNonScalableImageCheck(obj); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The hook returns |
||
|
|
||
| const isScalingAllowed = isAccessScalingAllowed && !hpaControlledScaling; | ||
|
|
||
|
|
@@ -126,15 +128,32 @@ export const PodRing: FC<PodRingProps> = ({ | |
| <SplitItem> | ||
| <Bullseye> | ||
| <div> | ||
| <Button | ||
| icon={<AngleUpIcon style={{ fontSize: '20' }} />} | ||
| type="button" | ||
| variant="plain" | ||
| aria-label={t('console-shared~Increase the Pod count')} | ||
| title={t('console-shared~Increase the Pod count')} | ||
| onClick={() => handleClick(1)} | ||
| isBlock | ||
| /> | ||
| {(() => { | ||
| const scaleUpButton = ( | ||
| <Button | ||
| icon={<AngleUpIcon style={{ fontSize: '20' }} />} | ||
| type="button" | ||
| variant="plain" | ||
| aria-label={t('console-shared~Increase the Pod count')} | ||
| title={t('console-shared~Increase the Pod count')} | ||
| onClick={() => handleClick(1)} | ||
| isBlock | ||
| /> | ||
| ); | ||
| // Show tooltip preemptively at >= 1 replica so users see | ||
| // the non-scalable warning before attempting to scale up | ||
| return isNonScalable && clickCount >= 1 ? ( | ||
| <Tooltip | ||
| content={t( | ||
| 'console-shared~This image is not intended to run with more than one replica. Scaling up may cause unexpected behavior.', | ||
| )} | ||
| > | ||
| {scaleUpButton} | ||
| </Tooltip> | ||
| ) : ( | ||
| scaleUpButton | ||
| ); | ||
| })()} | ||
| <Button | ||
| icon={<AngleDownIcon style={{ fontSize: '20' }} />} | ||
| type="button" | ||
|
|
||
| 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); | ||
| }); | ||
| }); |
| 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 }; | ||
| }; |
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.
235: "Unexpected behavior" = vague. users should expect what? how serious? if risks known (errors, data loss, conflicts...), name them. if not, SR: "Scaling up is not supported and might cause issues."