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

kubeadm: Introduce ValidateSupportedVersion #73474

Merged
merged 1 commit into from
Feb 2, 2019

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Jan 29, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Introduce ValidateSupportedVersion in place of DetectUnsupportedVersion

DetectUnsupportedVersion is somewhat uncomfortable, complex and inefficient function to use. It takes an entire YAML document as bytes, splits it up to byte slices of the different YAML sub-documents and group-version-kinds and searches through those to detect an unsupported kubeadm config. If such config is detected, the function returns an error, if it is not (i.e. the normal function operation) everything done so far is discarded.

This could have been acceptable, if not the fact, that in all cases that this function is called, the YAML document bytes are split up and an iteration on GVK map is performed yet again. Hence, we don't need DetectUnsupportedVersion in its current form as it's inefficient, complex and takes only YAML document bytes.

This change replaces DetectUnsupportedVersion with ValidateSupportedVersion, which takes a GroupVersion argument and checks if it is on the list of unsupported config versions. In that case an error is returned.
ValidateSupportedVersion relies on the caller to read and split the YAML document and then iterate on its GVK map checking if the particular GroupVersion is supported or not.

Which issue(s) this PR fixes:

Refs kubernetes/kubeadm#1296
Refs kubernetes/kubeadm#1084

Special notes for your reviewer:

This is a step towards simplifying the config APIs and making them more efficient.

Also, this is a step in introducing the concept of "deprecated config", which is supported config, that can be used at API level and as a source to migrate to the current config (via kubeadm config migrate), but not create a new cluster from it. This will be done in a future followup PR.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @fabriziopandini
/assign @timothysc

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 29, 2019
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

LGTM thanks @rosti
added one minor nit.

/priority important-longterm


// TODO: On our way to making the kubeadm API beta and higher, give good user output in case they use an old config file with a new kubeadm version, and
// tell them how to upgrade. The support matrix will look something like this now and in the future:
// IsSupportedVersion checks if a supplied GroupVersionKind is not on the list of unsupported GVKs. If it is, an error is returned.
Copy link
Member

Choose a reason for hiding this comment

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

this function does not validate Kind. does it make sense for it to accept GV (GroupVersion) instead of GVK?

i would also rename it to IsOldSupportedGroupVersion:

  • old because it only handles old versions
  • GroupVersion to indicate that it accepts GV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 for the name. Thanks for that! I was feeling kind of awkward with this name.
Yes, judging from the implementation, it is more logical for it to take GroupVersion, but for now every call to it has to be done with gvk.GroupVersion() as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the name probably has to be IsNotOldUnsupportedVersion. I seem to have been missing the old part of it.

Copy link
Member

@neolit123 neolit123 Jan 30, 2019

Choose a reason for hiding this comment

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

but for now every call to it has to be done with gvk.GroupVersion() as a parameter.

i think we have like 2 calls - one for test and one for init, so it seems fine.
we shouldn't have more, technically.

func IsSupportedVersion(gvk schema.GroupVersionKind) error {

IsSupportedOldVersion also works i guess.
but i think Is... usually implies that the function returns bool.
we probably should print the error on the caller size in inticonfiguration and make it return bool:
errors.Errorf("your configuration file uses an

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you are right here, but we need to be returned an error. This is a strictly utility function and it should not print directly to the user.
I can rename it to VerifyNotOldUnsupportedVersion and retain the error return. Verify prefixed funcs return errors (and not bools).

Copy link
Member

Choose a reason for hiding this comment

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

or just name it VerifySupportedVersion which returns an error if it's old and unsupported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can do too.

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 29, 2019
@rosti rosti changed the title kubeadm: Introduce IsSupportedVersion kubeadm: Introduce ValidateSupportedVersion Feb 1, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 1, 2019

@rosti: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws d13bd0d03205850fc48e2a92fdf617e1bdaf51b4 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks for the update @rosti

/skip
/approve

we can try getting a LGTM from someone else.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019
…rtedVersion

DetectUnsupportedVersion is somewhat uncomfortable, complex and inefficient
function to use. It takes an entire YAML document as bytes, splits it up to
byte slices of the different YAML sub-documents and group-version-kinds and
searches through those to detect an unsupported kubeadm config. If such config
is detected, the function returns an error, if it is not (i.e. the normal
function operation) everything done so far is discarded.

This could have been acceptable, if not the fact, that in all cases that this
function is called, the YAML document bytes are split up and an iteration on
GVK map is performed yet again. Hence, we don't need DetectUnsupportedVersion
in its current form as it's inefficient, complex and takes only YAML document
bytes.

This change replaces DetectUnsupportedVersion with ValidateSupportedVersion,
which takes a GroupVersion argument and checks if it is on the list of
unsupported config versions. In that case an error is returned.
ValidateSupportedVersion relies on the caller to read and split the YAML
document and then iterate on its GVK map checking if the particular
GroupVersion is supported or not.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@fabriziopandini
Copy link
Member

Thanks @rosti! well done!
/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, neolit123, rosti

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 merged commit 0c2613c into kubernetes:master Feb 2, 2019
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. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants