Skip to content

✨ Proposal: node reuse#162

Merged
metal3-io-bot merged 2 commits intometal3-io:masterfrom
Nordix:node-reuse-capm3
Mar 10, 2021
Merged

✨ Proposal: node reuse#162
metal3-io-bot merged 2 commits intometal3-io:masterfrom
Nordix:node-reuse-capm3

Conversation

@furkatgofurov7
Copy link
Copy Markdown
Member

@furkatgofurov7 furkatgofurov7 commented Feb 4, 2021

This PR proposes node reuse feature for the upgrade/remediation operations.

Related to: allow disabling disk cleaning
Co-authored by: @fmuyassarov

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

furkatgofurov7 commented Feb 4, 2021

/assign @dhellmann
/cc @maelk

@metal3-io-bot metal3-io-bot requested a review from maelk February 4, 2021 14:01
@dhellmann
Copy link
Copy Markdown
Member

dhellmann commented Feb 4, 2021

I'm shifting my focus a bit to an internal project, so it's probably best not to block designs like this on my review.

/unassign dhellmann
/assign zaneb

@metal3-io-bot metal3-io-bot assigned zaneb and unassigned dhellmann Feb 4, 2021
hardys
hardys reviewed Feb 8, 2021
View reviewed changes
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
andfasano
andfasano reviewed Feb 10, 2021
View reviewed changes
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
@fmuyassarov fmuyassarov force-pushed the node-reuse-capm3 branch 3 times, most recently from 6153e8c to 3021135 Compare February 12, 2021 10:00
@fmuyassarov
Copy link
Copy Markdown
Member

fmuyassarov commented Feb 12, 2021

/cc @maelk

@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Feb 22, 2021

@hardys @andfasano proposal has been updated, PTAL. Thanks!

andfasano
andfasano reviewed Feb 23, 2021
View reviewed changes
Comment thread design/cluster-api-provider-metal3/node_reuse.md
Comment thread design/cluster-api-provider-metal3/node_reuse.md
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
@fmuyassarov fmuyassarov force-pushed the node-reuse-capm3 branch 3 times, most recently from a86f3ef to 3cde1e9 Compare February 24, 2021 12:14
andfasano
andfasano reviewed Feb 25, 2021
View reviewed changes
Comment thread design/images/node_reuse.drawio Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
andfasano
andfasano reviewed Mar 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@andfasano andfasano left a comment

Choose a reason for hiding this comment

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

In general the main part of the proposal looks definitely in a better shape. I have added one question for a specific scenario that I think it could be worth to clarify, and some suggestions to improve the diagrams.

Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md
Comment thread design/cluster-api-provider-metal3/node_reuse.md Outdated
Comment thread design/cluster-api-provider-metal3/node_reuse.md
@furkatgofurov7 furkatgofurov7 requested a review from andfasano March 2, 2021 09:57
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 2, 2021

@andfasano the proposal has been updated and we addressed the latest comments(added the label removal step in the diagram). PTAL. Thank you!

andfasano
andfasano approved these changes Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@andfasano andfasano left a comment

Choose a reason for hiding this comment

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

The current proposal looks fine to me. Leaving to @hardys the final comments

maelk
maelk approved these changes Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maelk maelk left a comment

Choose a reason for hiding this comment

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

You should specify that md-pool1 is for the machine deployment named pool1, and if it was KCP, it would be kcp-pool1 . This is not clear, and could be confusing if md and KCP have same names otherwise

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Mar 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, furkatgofurov7, maelk

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 Mar 2, 2021
@furkatgofurov7
Copy link
Copy Markdown
Member Author

furkatgofurov7 commented Mar 2, 2021

You should specify that md-pool1 is for the machine deployment named pool1, and if it was KCP, it would be kcp-pool1 . This is not clear, and could be confusing if md and KCP have same names otherwise

Thanks for review, updated.

hardys
hardys reviewed Mar 4, 2021
View reviewed changes
Comment thread design/images/node_reuse_flow.svg
@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 4, 2021

The current proposal looks fine to me. Leaving to @hardys the final comments

This version looks good to me, thanks! One nit re the inlined SVG, that would perhaps be better as a separate file?

Otherwise happy for this to merge, but we agreed a few days of lazy consensus in the meeting yesterday.

@fmuyassarov
Copy link
Copy Markdown
Member

fmuyassarov commented Mar 4, 2021 •
edited
Loading

The current proposal looks fine to me. Leaving to @hardys the final comments

This version looks good to me, thanks! One nit re the inlined SVG, that would perhaps be better as a separate file?

Otherwise happy for this to merge, but we agreed a few days of lazy consensus in the meeting yesterday.

Thanks. As agreed yesterday in community meeting we will have lazy consensus until next community meeting on Wednesday (10.03) to give time for others to review too.

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 10, 2021

Lazy consensus period completed and agreed to merge in the community meeting today.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2021
@metal3-io-bot metal3-io-bot merged commit 0f1c05b into metal3-io:master Mar 10, 2021
@furkatgofurov7 furkatgofurov7 mentioned this pull request Mar 22, 2021
Merged
@furkatgofurov7 furkatgofurov7 deleted the node-reuse-capm3 branch April 30, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hardys hardys hardys left review comments

+3 more reviewers

@fmuyassarov fmuyassarov fmuyassarov left review comments

@maelk maelk maelk approved these changes

@andfasano andfasano andfasano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zaneb zaneb

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@furkatgofurov7 @dhellmann @fmuyassarov @metal3-io-bot @hardys @maelk @andfasano @zaneb