Skip to content

✨ Add nodeReuse feature#169

Merged
metal3-io-bot merged 1 commit into
metal3-io:masterfrom
Nordix:node-reuse/furkat
May 7, 2021
Merged

✨ Add nodeReuse feature#169
metal3-io-bot merged 1 commit into
metal3-io:masterfrom
Nordix:node-reuse/furkat

Conversation

@furkatgofurov7
Copy link
Copy Markdown
Member

@furkatgofurov7 furkatgofurov7 commented Mar 1, 2021 •
edited
Loading

What this PR does / why we need it:
Adds node reuse feature in CAPM3 machine controller.
Implementation of node reuse proposal

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 1, 2021
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 1, 2021

/hold
Tests and webhook conversions are missing.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2021
@furkatgofurov7 furkatgofurov7 force-pushed the node-reuse/furkat branch 4 times, most recently from 09ac7ba to 8135b9e Compare March 9, 2021 17:59
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 9, 2021

/retest

@furkatgofurov7 furkatgofurov7 force-pushed the node-reuse/furkat branch 3 times, most recently from 9bd2b42 to f129327 Compare March 10, 2021 11:02
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 10, 2021

/test generate

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 10, 2021

/retest

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 10, 2021

/retest

namnx228
namnx228 suggested changes Mar 10, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@namnx228 namnx228 left a comment

Choose a reason for hiding this comment

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

Great work!!! Something can be improved that can be found in the comments

Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 10, 2021

/retest

We have to fix golint(take possible fix #176 in? ), otherwise it has to be re-run each and every time:/

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 11, 2021

/retitle Add nodeReuse feature

@metal3-io-bot metal3-io-bot changed the title ✨ [WIP] Add Node reuse feature Add nodeReuse feature Mar 11, 2021
fmuyassarov
fmuyassarov reviewed Mar 14, 2021
View reviewed changes
Comment thread api/v1alpha4/metal3machinetemplate_types.go
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread config/crd/bases/infrastructure.cluster.x-k8s.io_metal3machinetemplates.yaml Outdated
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 15, 2021

/retest

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 15, 2021

/retest

@furkatgofurov7 furkatgofurov7 force-pushed the node-reuse/furkat branch 2 times, most recently from 1b46938 to a95b343 Compare March 15, 2021 17:25
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 15, 2021

/retest

@furkatgofurov7 furkatgofurov7 force-pushed the node-reuse/furkat branch 4 times, most recently from 2284a23 to 64f694b Compare April 18, 2021 08:48
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Apr 18, 2021

/unhold
/test-integration

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2021
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Apr 18, 2021

/test-integration

@furkatgofurov7 furkatgofurov7 requested a review from maelk April 19, 2021 08:15
namnx228
namnx228 suggested changes Apr 30, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@namnx228 namnx228 left a comment

Choose a reason for hiding this comment

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

Just some nits. Otherwise, it looks good to me

Comment thread baremetal/metal3machine_manager.go
Comment thread baremetal/metal3machine_manager.go Outdated
@fmuyassarov
Copy link
Copy Markdown
Member

fmuyassarov commented May 2, 2021

/retest

1 similar comment
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented May 3, 2021

/retest

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented May 3, 2021

/test-integration

kashifest
kashifest reviewed May 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve
overall looks good.
I have few small nits.
There are places where code entropy could have been reduced specially codes related to KCP and MD search are pretty much redundant, but this is cosmetic change.
Squash the commits please.

Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3data_manager.go
Comment thread baremetal/metal3machine_manager.go
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machine_manager.go
host.OwnerReferences = hostOwnerReferences

// Delete nodeReuseLabelName from host
m.Log.Info("Deleting nodeReuseLabelName from host, if any")
Copy link
Copy Markdown
Member

@kashifest kashifest May 3, 2021

Choose a reason for hiding this comment

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

why are we deleting the label in setHostConsumerRef ?

Copy link
Copy Markdown
Member Author

@furkatgofurov7 furkatgofurov7 May 3, 2021

Choose a reason for hiding this comment

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

This was chosen as 'best' time to remove the label while using node reuse feature because once we set consumerRef which means M3M will be linked to BMH with nodeReuse label, and so we do not need the label anymore. Do you foresee any problems with that @kashifest ?

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented May 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7, kashifest

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

The pull request process is described here

Details 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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2021
@furkatgofurov7 furkatgofurov7 force-pushed the node-reuse/furkat branch 2 times, most recently from d204323 to 4bad3e8 Compare May 3, 2021 12:03
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented May 3, 2021

@namnx228 @kashifest thanks for the reviews/feedbacks, I have addressed/answered most of them. PTAL again.

/test-integration

@namnx228
Copy link
Copy Markdown
Member

namnx228 commented May 5, 2021

LGTM

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented May 5, 2021

Squash the commits please.

Squashing done.

/test-integration

@furkatgofurov7 furkatgofurov7 mentioned this pull request May 6, 2021
Merged
@jan-est
Copy link
Copy Markdown
Contributor

jan-est commented May 7, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2021
@metal3-io-bot metal3-io-bot merged commit cc0e412 into metal3-io:master May 7, 2021
@furkatgofurov7 furkatgofurov7 mentioned this pull request May 23, 2021
Merged
@kashifest kashifest deleted the node-reuse/furkat branch June 20, 2022 06:31
honza pushed a commit to honza/cluster-api-provider-metal3 that referenced this pull request Jan 27, 2025
@openshift-merge-robot
Merge pull request metal3-io#169 from hardys/owners_update
22d1cd6 
Update OWNERS file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kashifest kashifest kashifest left review comments

@andfasano andfasano Awaiting requested review from andfasano

@hardys hardys Awaiting requested review from hardys

@maelk maelk Awaiting requested review from maelk

+3 more reviewers

@namnx228 namnx228 namnx228 requested changes

@fmuyassarov fmuyassarov fmuyassarov left review comments

@jan-est jan-est jan-est left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@maelk maelk

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@furkatgofurov7 @fmuyassarov @metal3-io-bot @namnx228 @jan-est @maelk @kashifest