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.

Sign up for GitHub

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

Jump to bottom

add Deprovision action to Bare Metal Hosts #3811

Merged
merged 1 commit into from Jan 22, 2020
Merged

add Deprovision action to Bare Metal Hosts #3811

merged 1 commit into from Jan 22, 2020

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Dec 19, 2019 •
edited

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2019
@atiratree atiratree mentioned this pull request Dec 19, 2019
Merged
@atiratree
Copy link
Member Author

aaa

depr

@jtomasek @honza

@openshift-ci-robot openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/shared Related to console-shared labels Dec 19, 2019
@atiratree
Copy link
Member Author

/retest

jtomasek
jtomasek suggested changes Jan 3, 2020
View reviewed changes
frontend/packages/metal3-plugin/src/k8s/requests/bare-metal-host.ts Outdated
@@ -13,3 +17,22 @@ export const createBareMetalHost = async (bareMetalHost, secret) => [
await k8sCreate(SecretModel, secret),
await k8sCreate(BareMetalHostModel, bareMetalHost),
];

export const deprovision = async (machine: MachineKind, machineSet: MachineSetKind) => {
Copy link

Choose a reason for hiding this comment

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

machineset is optional here:
If host's machine is managed by machineSet -> annotate the machine and scale down the machineset to trigger deprovisioning.
if host's machine is not managed by machineSet (e.g. master) -> just delete the machine to start deprovisioning.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

I am still adding the annotation to distinguish the machines for hiding the deprovision action

@atiratree atiratree force-pushed the metal.deprovisionAction branch 2 times, most recently from 1c6a7d6 to b9cc608 Compare January 6, 2020 16:57
@atiratree
Copy link
Member Author

/retest

@atiratree
Copy link
Member Author

#3810 merged, rebased

jtomasek
jtomasek suggested changes Jan 13, 2020
View reviewed changes
frontend/packages/metal3-plugin/src/k8s/requests/bare-metal-host.ts
new PatchBuilder('/metadata/annotations')
.setObjectUpdate(DELETE_MACHINE_ANNOTATION, 'true', getAnnotations(machine))
.build(),
]);
Copy link

Choose a reason for hiding this comment

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

I believe this should go inside (replicas > 0) { block (Line 37) as it is needed only in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is safer to specify the intent of removing this machine in all cases (shouldn't hurt anything)

frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-menu-actions.tsx
return {
hidden:
!machine ||
!!getAnnotations(machine, {})[DELETE_MACHINE_ANNOTATION] ||
Copy link

Choose a reason for hiding this comment

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

It should be ok to run deprovision action even when the annotation is already there (to for example cover the case when the machine was annotated but the machineset failed to scale down).

Copy link

Choose a reason for hiding this comment

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

Instead, we should prevent re-triggering if host is already in 'deprovisioning' status. It seems that the machine is deleted only after the hosts image has been removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be a bug on the machineSet side. I think it is much safer to rely on the annotation because the status can take a while to get updated and the user might remove two machines by accident (will randomly kill next one).

I added a safety fallback to the request so the annotation doesn't get stuck when the machine set patch fails

frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-menu-actions.tsx
hidden:
!machine ||
!!getAnnotations(machine, {})[DELETE_MACHINE_ANNOTATION] ||
(getMachineMachineSetOwner(machine) && !machineSet),
Copy link

Choose a reason for hiding this comment

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

The machineSet is only populated if it is the owner of the machine (in BareMetalHostsPage and menuActionsCreator), so this line should not be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but this line is here for the loading. So that the action will stay hidden until we load the machine set of a machine which should have one.

frontend/packages/metal3-plugin/src/selectors/machine.ts
import { MachineSetModel } from '@console/internal/models';
import { getOwnerReferences } from '@console/shared/src';

export const getMachineMachineSetOwner = (machine: MachineKind) => {
Copy link

Choose a reason for hiding this comment

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

This would be beetter to add into @console/shared/src/selectors/machine.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather move this in a followup once #4014 merges and make it more generic

frontend/packages/metal3-plugin/src/selectors/machine.ts
kind: MachineSetModel.kind,
} as any;
return (getOwnerReferences(machine) || []).find((reference) =>
compareOwnerReference(desiredReference, reference, true),
Copy link

Choose a reason for hiding this comment

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

@console/metal3-plugin nor @console/shared should import from '@console/kubevirt-plugin'

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right: #4014

frontend/packages/metal3-plugin/src/types/misc.ts Outdated
import { K8sResourceCommon, K8sResourceKind } from '@console/internal/module/k8s';
import { FirehoseResult } from '@console/internal/components/utils';

export type OptionalFirehoseResult<
Copy link

Choose a reason for hiding this comment

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

Maybe this could be put somewhere near the FirehoseResult definition instead of metal3-plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

#4016

This was referenced Jan 20, 2020
Merged
Merged
@jtomasek
Copy link

/retest

@atiratree
Copy link
Member Author

rebased and fixed

@jtomasek
Copy link

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, suomiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020
@jelkosz
Copy link

jelkosz commented Jan 21, 2020

/test e2e-gcp-console

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@atiratree
Copy link
Member Author

/retest

1 similar comment
@atiratree
Copy link
Member Author

/retest

@spadgett
Copy link
Member

/hold
the merge queue is blocked

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c728d85 into openshift:master Jan 22, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtomasek jtomasek jtomasek requested changes

@rhrazdil rhrazdil Awaiting requested review from rhrazdil

Assignees

@jtomasek jtomasek

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@atiratree @jtomasek @openshift-ci-robot @jelkosz @openshift-bot @spadgett @openshift-merge-robot @suomiy