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: add final fallback to constants.CurrentKubernetesVersion #72454

Merged
merged 1 commit into from Mar 27, 2019

Conversation

@rojkov
Copy link
Member

commented Dec 31, 2018

What this PR does / why we need it:

It may happen that both the git version and the remote version
are broken/inaccessible. In this case the broken remote version
would be used.

To overcome this situation fall back to the constant CurrentKubernetesVersion.

The alternative could be os.Exit(1).

Also this change fixes bazel-based unit tests in air-gapped environment.

What type of PR is this?

/kind cleanup
/kind failing-test

Does this PR introduce a user-facing change?:

In case kubeadm can't access the current Kubernetes version remotely and fails to parse
the git-based version it falls back to a static predefined value of
k8s.io/kubernetes/cmd/kubeadm/app/constants.CurrentKubernetesVersion.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

Hi @rojkov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

/cc @rosti
/cc @neolit123
/cc @bart0sh

@k8s-ci-robot k8s-ci-robot requested review from bart0sh, neolit123 and rosti Dec 31, 2018

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

/kind cleanup
/kind failing-test

@bart0sh

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

/ok-to-test
/priority important-longterm

klog.Warningf("could not obtain client version; using remote version: %s", body)
return KubernetesReleaseVersion(body)
}

if err != nil && clientVersionErr != nil {
klog.Warningf("could not obtain neither client nor remote version; fall back to: %s", constants.CurrentKubernetesVersion)
return KubernetesReleaseVersion(constants.CurrentKubernetesVersion.String())

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 31, 2018

Member

is there a unit test to ensure this constant is always correct? if not, it is very likely to drift and be forgotten

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 1, 2019

Member

after the resent refactor these should be the only constants that we bump on each minor release and we must bump them anyway:

// MinimumControlPlaneVersion specifies the minimum control plane version kubeadm can deploy
MinimumControlPlaneVersion = version.MustParseSemantic("v1.12.0")
// MinimumKubeletVersion specifies the minimum version of kubelet which kubeadm supports
MinimumKubeletVersion = version.MustParseSemantic("v1.12.0")
// CurrentKubernetesVersion specifies current Kubernetes version supported by kubeadm
CurrentKubernetesVersion = version.MustParseSemantic("v1.13.0")

having a unit test puts this in a weird space - i.e. to which value to compare the constant to?
the constant is a fallback: remote version -> client version -> constant version.

  1. we can potentially compare to the remote against https://dl.k8s.io/release/stable-1.txt (with patch=0), but arguably unit tests should not depend on remote connectivity....we do that already in a few places.
  2. compare to the client version. problem here is that unit tests do not receive the version as ldflags.
    i think this might be a problem only for the integration tests:
    https://github.com/kubernetes/kubernetes/blob/master/hack/make-rules/test-kubeadm-cmd.sh

leaving the final call to @timothysc and @kad on this PR.
/assign @timothysc
/assign @kad

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 1, 2019

Member

these should be the only constants that we bump on each minor release and we must bump them anyway

Doesn't the current release constant require bumping on every patch version as well?

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 1, 2019

Member

CurrentKubernetesVersion parses to a MAJOR.MINOR.PATCH object, but the important elements are MAJOR.MINOR.

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 1, 2019

Member

That seems like a misleading constant, then, and one that would be likely to be used incorrectly.

This comment has been minimized.

Copy link
@rojkov

rojkov Jan 2, 2019

Author Member

Honestly speaking this recursive function with side effects smells suboptimal to me since it clearly violates the Single Responsibility principle. I would rather try to constrain ClusterConfig.KubernetesVersion to be a canonical version only and always. Then at config parsing/cmdline option setting time we'd detect whether the provided value is

  1. a version or
  2. just a version label supposed to be resolved remotely into a version or
  3. nil.

In the first case normalizedBuildVersion() would be used once. In the second case KubernetesReleaseVersion() would be limited to resolving the remote version only. And in the third case we could try to read the gitVersion and fail if it's not set.

Then the only authoritative source of the version value would be ClusterConfig.KubernetesVersion. With this approach the unit tests would not touch networking.

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 2, 2019

Member

Honestly speaking this recursive function with side effects smells suboptimal to me since it clearly violates the Single Responsibility principle

i support your concern here and the recursion can be avoided.
@kad and @luxas probably have opinions here as they authored the original?

I would rather try to constrain ClusterConfig.KubernetesVersion to be a canonical version only and always

isn't this the case already? if the user does not feed a KubernetesVersion and a config, the version has to come from somewhere. internet endpoint, client version, constant?

With this approach the unit tests would not touch networking.

there are calls like this one:

// KubernetesVersion is not used, but we set it explicitly to avoid the lookup
// of the version from the internet when executing ConfigFileAndDefaultsToInternalConfig
phaseutil.SetKubernetesVersion(cfg)

that pre-set a version so that the version is later not fetched from the internet for commands that don't need internet. but this still needs a client that was built properly.

so potentially, unit tests can do the same.

This comment has been minimized.

Copy link
@rojkov

rojkov Jan 2, 2019

Author Member

isn't this the case already? if the user does not feed a KubernetesVersion and a config, the version has to come from somewhere. internet endpoint, client version, constant?

IIRC some unit tests don't (can't) consult ClusterConfig.KubernetesVersion relying solely on KubernetesReleaseVersion(). I'll try to come up with a proposal.

This comment has been minimized.

Copy link
@rojkov

rojkov Jan 2, 2019

Author Member

... in a separate PR that is.

klog.Warningf("could not obtain client version; using remote version: %s", body)
return KubernetesReleaseVersion(body)
}

if err != nil && clientVersionErr != nil {

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 1, 2019

Member

why not unify the err != nil and err == nil branches under a clientVersionErr != nil branch?

This comment has been minimized.

Copy link
@rojkov

rojkov Jan 2, 2019

Author Member

I moved the branches under clientVersionErr != nil.

klog.Warningf("could not obtain client version; using remote version: %s", body)
return KubernetesReleaseVersion(body)
}

if err != nil && clientVersionErr != nil {
klog.Warningf("could not obtain neither client nor remote version; fall back to: %s", constants.CurrentKubernetesVersion)
return KubernetesReleaseVersion(constants.CurrentKubernetesVersion.String())

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 1, 2019

Member

after the resent refactor these should be the only constants that we bump on each minor release and we must bump them anyway:

// MinimumControlPlaneVersion specifies the minimum control plane version kubeadm can deploy
MinimumControlPlaneVersion = version.MustParseSemantic("v1.12.0")
// MinimumKubeletVersion specifies the minimum version of kubelet which kubeadm supports
MinimumKubeletVersion = version.MustParseSemantic("v1.12.0")
// CurrentKubernetesVersion specifies current Kubernetes version supported by kubeadm
CurrentKubernetesVersion = version.MustParseSemantic("v1.13.0")

having a unit test puts this in a weird space - i.e. to which value to compare the constant to?
the constant is a fallback: remote version -> client version -> constant version.

  1. we can potentially compare to the remote against https://dl.k8s.io/release/stable-1.txt (with patch=0), but arguably unit tests should not depend on remote connectivity....we do that already in a few places.
  2. compare to the client version. problem here is that unit tests do not receive the version as ldflags.
    i think this might be a problem only for the integration tests:
    https://github.com/kubernetes/kubernetes/blob/master/hack/make-rules/test-kubeadm-cmd.sh

leaving the final call to @timothysc and @kad on this PR.
/assign @timothysc
/assign @kad

@rojkov rojkov force-pushed the rojkov:kubeadm-fallback-version branch from b500a95 to 8ead090 Jan 2, 2019

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

/test pull-kubernetes-integration

@kad

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

/lgtm
for this PR.
As for recursion: at the time this function was written, it had single responsibility of resolving label to exact validated version string, with support for label->label->..->version resolving (see tests for this function). Later additions with different types of fallbacks complicated it unfortunately.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 11, 2019

@kad

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

BTW, as this has user visible behavior change (static default fallback version), it would be good to have release note line.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

BTW, as this has user visible behavior change (static default fallback version), it would be good to have release note line.

Thanks! I've updated the release note.

@rosti

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

For feedback and approval
/assign @neolit123 @fabriziopandini

return KubernetesReleaseVersion(clientVersion)
if clientVersionErr == nil {
// Handle air-gapped environments by falling back to the client version.
klog.Infof("could not fetch a Kubernetes version from the internet: %v", err)

This comment has been minimized.

Copy link
@rosti

rosti Jan 17, 2019

Member

Technically speaking, this one should be a warning.

This comment has been minimized.

Copy link
@rojkov

rojkov Jan 17, 2019

Author Member

Agree. Made them warnings now.

kubeadm: add final fallback to constants.CurrentKubernetesVersion
It may happen that both the git version and the remote version
are broken/inaccessible. In this case the broken remote version
would be used.

To overcome this situation fall back to the constant CurrentKubernetesVersion.

The alternative could be os.Exit(1).

Also this change fixes Bazel-based unit tests in air-gapped environment.

@rojkov rojkov force-pushed the rojkov:kubeadm-fallback-version branch from 8ead090 to 9e25a00 Jan 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 17, 2019

@fabriziopandini
Copy link
Member

left a comment

@rojkov first of all sorry for the delay in reviewing this PR.

As per slack discussion, I understood that adding a third fallback rule for version discovery is a necessary evil in for following us cases:

  • git version is broken when building unit tests with Bazel behind proxy (Bazel drops http_proxy= vars)
  • running a build from the source tarball (not from git) in an air gaped environment

The proposed fallback method is not perfect, because it will always point at the .0 patch of each minor, but I think it is acceptable for the above use cases (running unit tests).

However, in my humble opinion, this function is already too complicated and I think that we cannot insert additional complexity without cleaning up some technical debt.
What do you think about the following proposal?

  • KubernetesReleaseVersion should be given the only the responsibility to coordinate the fallback logic, something similar to
getKubernetesReleaseVersion (from internet)
if ok return 

getGitVersion
if ok return

use constant
  • getKubernetesReleaseVersion should have the single responsibility of resolving label to exact validated version string, with support for label->label->..->version resolving (as per @kad comment)

The expected benefit of the above proposal is to spilt the fallback logic from the label resolving/recursive logic, thus hopefully improving code readability/maintenabily/testability.

I'm going also to open a separated issue for improving the name of CurrentKubernetesVersion as per @liggitt comment

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

@fabriziopandini Thank you for the review! Actually I've already rewritten the function in this commit rojkov@d991754

    kubeadm: refactor Kubernetes release version calculations
    
    Move version label resolution logic to the dedicated
    function `resolveVersionLabel()`. This way we
    
    1. get rid of recursion in `KubernetesReleseVersion()`;
    2. guaranty that the function never falls into endless
       loops in case the server mistakenly resolves "stable"
       into "stable-1" and "stable-1" back into "stable".
    3. get simpler code in `KubermetesReleaseVersion`.
    
    Also it renames `normalizedBuildVersion()` to
    `normilizedBuildVersion()` to make it sound like a verb.
    Now the function clearly indicates errors instead of making
    a developer analize its output.
    
    Detection of air-gapped environment is done through examining
    http.Client's error rather than output of `fetchFromURL()` which
    depends the function's internal logic.

Basically it implements your suggestion already. I wanted to rebase it on top of this PR, but I can do the other way around.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

Submitted #73129 which is pending @kad 's review. Then I'll rebase this PR.

@fabriziopandini

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

/hold
Waiting for #73129 to land before

@kad

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@fabriziopandini why to hold this one? I think it can be merged.

@timothysc

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

/hold cancel

@timothysc

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 27, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rojkov, timothysc

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 5a6c66a into kubernetes:master Mar 27, 2019

18 checks passed

cla/linuxfoundation rojkov authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Context retired without replacement.
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.