Skip to content
This repository was archived by the owner on Oct 10, 2023. It is now read-only.

Add MachineDeployment specifier to node pool#1074

Merged
tenczar merged 1 commit intovmware-tanzu:mainfrom
tenczar:explicit_md_node_pool
Nov 9, 2021
Merged

Add MachineDeployment specifier to node pool#1074
tenczar merged 1 commit intovmware-tanzu:mainfrom
tenczar:explicit_md_node_pool

Conversation

@tenczar
Copy link
Copy Markdown
Contributor

@tenczar tenczar commented Nov 6, 2021 •
edited by randomvariable
Loading

Adds a command line flag to specify the MachineDeployment to use as a
base when creating a new node pool

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #1067

Describe testing done for PR

Release note

Adds a new flag to cluster node-pool set `--machine-deployment-base` This flag allows a user to specify an explicit MachineDeployment as the base from which a new node pool will be created.

PR Checklist

  • Squash the commits into one or a small number of logical commits
  • Use good commit messages
  • Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

Special notes for your reviewer

@tenczar tenczar requested a review from a team as a code owner November 6, 2021 00:30
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 6, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211106003814/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

imikushin
imikushin reviewed Nov 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@imikushin imikushin left a comment

Choose a reason for hiding this comment

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

One question. Also, can we add a test for this? (e.g. when mdBase is specified, and 2 cases: when one exists, and when one doesn't)

@randomvariable
Copy link
Copy Markdown
Contributor

randomvariable commented Nov 6, 2021

@nickperry PTAL

@nickperry
Copy link
Copy Markdown

nickperry commented Nov 6, 2021

@nickperry PTAL

LGTM

@tenczar tenczar force-pushed the explicit_md_node_pool branch 2 times, most recently from 2abb840 to 4cf5800 Compare November 8, 2021 22:31
@tenczar tenczar requested a review from imikushin November 8, 2021 22:38
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 8, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211108224000/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 8, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211108224109/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@tenczar tenczar force-pushed the explicit_md_node_pool branch from 4cf5800 to cfb16a4 Compare November 8, 2021 22:44
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 8, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211108225517/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

prkalle
prkalle reviewed Nov 8, 2021
View reviewed changes
imikushin
imikushin approved these changes Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@imikushin imikushin left a comment

Choose a reason for hiding this comment

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

LGTM

@imikushin imikushin added the ok-to-merge PRs should be labelled with this before merging label Nov 8, 2021
prkalle
prkalle approved these changes Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@prkalle prkalle left a comment

Choose a reason for hiding this comment

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

LGTM with one minor nit comment

@tenczar tenczar force-pushed the explicit_md_node_pool branch from cfb16a4 to 7deac5c Compare November 8, 2021 23:46
saji-pivotal
saji-pivotal approved these changes Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@saji-pivotal saji-pivotal left a comment

Choose a reason for hiding this comment

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

One tiny nit. But LGTM otherwise

pkg/v1/tkg/client/machine_deployment.go Outdated
Volumes *[]tkgsv1alpha2.Volume `yaml:"volumes,omitempty"`
TKR tkgsv1alpha2.TKRReference `yaml:"tkr,omitempty"`
NodeDrainTimeout *metav1.Duration `yaml:"nodeDrainTimeout,omitempty"`
MachineDeploymentBase string `yaml:"machineDeploymentBase,omitempty"`
Copy link
Copy Markdown
Contributor

@saji-pivotal saji-pivotal Nov 8, 2021

Choose a reason for hiding this comment

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

nit: Can this be renamed to BaseMachineDeployment or BaseMachineDeploymentName ?

Copy link
Copy Markdown
Contributor Author

@tenczar tenczar Nov 8, 2021

Choose a reason for hiding this comment

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

What about the command line flag?

Copy link
Copy Markdown
Contributor

@saji-pivotal saji-pivotal Nov 9, 2021

Choose a reason for hiding this comment

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

how about base-machine-deployment ?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 8, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211108235951/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@tenczar
Add MachineDeployment specifier to node pool
2cbc63a 
Adds a command line flag to specify the MachineDeployment to use as a
base when creating a new node pool
@tenczar tenczar force-pushed the explicit_md_node_pool branch from 7deac5c to 2cbc63a Compare November 9, 2021 01:01
saji-pivotal
saji-pivotal approved these changes Nov 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@saji-pivotal saji-pivotal left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 9, 2021

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1074/20211109011904/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@tenczar tenczar merged commit c1d8f77 into vmware-tanzu:main Nov 9, 2021
@tenczar tenczar deleted the explicit_md_node_pool branch November 9, 2021 02:27
yharish991 pushed a commit to yharish991/tanzu-framework that referenced this pull request Nov 9, 2021
@tenczar @yharish991
Add MachineDeployment specifier to node pool (vmware-tanzu#1074)
d56decc 
Adds a command line flag to specify the MachineDeployment to use as a
base when creating a new node pool
@rajaskakodkar rajaskakodkar mentioned this pull request Nov 22, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers

@randomvariable randomvariable randomvariable left review comments

@imikushin imikushin imikushin approved these changes

@prkalle prkalle prkalle approved these changes

@saji-pivotal saji-pivotal saji-pivotal approved these changes

@swalner-vmware swalner-vmware swalner-vmware approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-not-required ok-to-merge PRs should be labelled with this before merging

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

A dangerous assumption is made when creating a new machine deployment that the first MD to be returned can be used as the base

8 participants

@tenczar @randomvariable @nickperry @imikushin @prkalle @saji-pivotal @swalner-vmware @vmwclabot