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.

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

gke deployer supports creating GKE Autopilot clusters #129

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

chizhg
Copy link
Contributor

@chizhg chizhg commented Apr 16, 2021

gke deployer supports creating GKE Autopilot clusters.

GKE Autopilot creation command only supports a subset of args of the general cluster creation command, see https://cloud.google.com/sdk/gcloud/reference/container/clusters/create-auto.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 16, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 16, 2021
@chizhg
Copy link
Contributor Author

chizhg commented Apr 16, 2021

/cc @amwat

@k8s-ci-robot k8s-ci-robot requested a review from amwat April 16, 2021 19:07
Comment on lines 83 to 89
// A few args are not supported in GKE Autopilot cluster creation, so they should be left unset
// when they are not provided via the flags.
// https://cloud.google.com/sdk/gcloud/reference/container/clusters/create-auto
if d.machineType != "" {
args = append(args, "--machine-type="+d.machineType)
}
if d.nodes != 0 {
args = append(args, "--num-nodes="+strconv.Itoa(d.nodes))
}
if d.imageType != "" {
args = append(args, "--image-type="+d.imageType)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC the current changes only fixes assumptions while generating the flags , but users still need to use the power "create-command" override?

I think we should just simply support autopilot first class, by adding an auto pilot option?

kubetest2 gke --up --autopilot

and move this validation/generation to verifyUpFlags()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered about this option before making the change. The question is how we should handle the case when the user provides both --autopilot and --create-command=container clusters create xxx yyy flags. If we throw an error, that is even worse than simply relying on "create-command" override. If we replace create with create-auto, that could confuse users..

I think the best way to support this is breaking down --create-command into three flags:
--gcloud-command-version: can be alpha, beta, or default
--autopilot: can be true or false
--gcloud-extra-flags: can be a string of extra flags we want to add to the gcloud command
But since kubetest2 is not versioned now, we'll probably need to consider backward compatibility when we make the change.

WDYT? Any better suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall I'd want to see a state where if create-command is specified everything else should be ignored (well super ideally this doesn't need to exist, but it's almost impossible to cover all cases so we keep it for power users) .

Relying on a mix of both cluster creation flags of the kubetest2 tool and the custom create command definitely is not great.

--gcloud-command-version: can be alpha, beta, or default
--autopilot: can be true or false
--gcloud-extra-flags: can be a string of extra flags we want to add to the gcloud command

This approach sounds reasonable to me. I don't think we need to worry about backwards compatibility for new flags? (we'll still keep the create-command flag around)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They need to be mutual exclusive since --create-command can be configured to include all the 3 new flags, so I added a logic to ignore --create-command if any of these 3 new flags are explicitly specified. Please let me know if it looks good to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we invert it? let's treat --create-command as an explicit override and ignore anything else, and can update the flag help text accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chizhg chizhg force-pushed the gke-autopilot branch 4 times, most recently from 753d043 to 3f97d4c Compare April 23, 2021 22:39
Copy link
Contributor

@amwat amwat left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

args = append(args, "--machine-type="+d.machineType)
args = append(args, "--num-nodes="+strconv.Itoa(d.nodes))
args = append(args, "--image-type="+d.imageType)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth refactoring all the argument builder logic somewhere as a follow up.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, chizhg

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1c731a5 into kubernetes-sigs:master Apr 23, 2021
@chizhg chizhg deleted the gke-autopilot branch April 24, 2021 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants