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

Remove rktnetes code #61432

Merged
merged 3 commits into from Mar 29, 2018

Conversation

@filbranden
Member

filbranden commented Mar 20, 2018

What this PR does / why we need it:
rktnetes is scheduled to be deprecated in 1.10 (#53601). According to the deprecation policy for beta CLI and flags, we can remove the feature in 1.11.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #58721

Special notes for your reviewer:

Release note:

Removed rknetes code, which was deprecated in 1.10.

/assign @yujuhong
/hold

Hold until the end of the freeze.

@dashpole

This comment has been minimized.

Contributor

dashpole commented Mar 20, 2018

/ok-to-test

@dashpole

This comment has been minimized.

Contributor

dashpole commented Mar 20, 2018

/hold cancel
It wont merge until freeze is over anyways

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 20, 2018

It wont merge until freeze is over anyways

Just being extra extra careful... 😁

Thanks for triggering the tests!

@yujuhong

Is there anything in the Godep that we should clean up now that the entire kubelet/rkt package is gone?

fs.MarkDeprecated("rkt-api-endpoint", "will be removed in a future version. Rktnetes has been deprecated in favor of rktlet (https://github.com/kubernetes-incubator/rktlet).")
fs.StringVar(&s.RktStage1Image, "rkt-stage1-image", s.RktStage1Image, "image to use as stage1. Local paths and http/https URLs are supported. If empty, the 'stage1.aci' in the same directory as '--rkt-path' will be used.")
fs.MarkDeprecated("rkt-stage1-image", "will be removed in a future version. Rktnetes has been deprecated in favor of rktlet (https://github.com/kubernetes-incubator/rktlet).")
// Rkt-specific settings: These are here for backwards compatibility, but rkt support has been removed.

This comment has been minimized.

@yujuhong

yujuhong Mar 21, 2018

Contributor

Hm... I think we should just remove these flags. If users are still setting those, it's a serious issue because the rkt is no longer supported. Kubelet should fail loudly.

This comment has been minimized.

@filbranden

filbranden Mar 21, 2018

Member

Done, removed those. Makes sense.

}
// rktnetes cannot be run with CRI.
if containerRuntime != kubetypes.RktContainerRuntime {

This comment has been minimized.

@yujuhong

yujuhong Mar 21, 2018

Contributor

I assume you simply remove the if? It's hard to tell from looking at the diff.

This comment has been minimized.

@dashpole

dashpole Mar 21, 2018

Contributor

It is much easier to see if you ignore whitespace

This comment has been minimized.

@filbranden

filbranden Mar 21, 2018

Member

Yes, indeed better asking GitHub to ignore whitespace when looking at the diff.

To answer your question: Yes, only the "if" was removed, and indentation adjusted.

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 21, 2018

Is there anything in the Godep that we should clean up now that the entire kubelet/rkt package is gone?

I'll take a look at Godeps and see if there are dependencies that go away...

Should we do this in this same PR or in a follow up?

There are also some references to RKT_* variables under cluster/, but I'd like to deal with those in a follow up, in order to reduce risk.

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 21, 2018

Is there anything in the Godep that we should clean up now that the entire kubelet/rkt package is gone?

I'll take a look at Godeps and see if there are dependencies that go away...

Hi @yujuhong,

Is there a procedure to maintain/update the Godeps?

I didn't find a script under hack/ that would update it or re-generate it... Do I need to save/restore? If you could point me at a documentation for the steps used to maintain that, that'd be great!

Thanks,
Filipe

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 22, 2018

Is there anything in the Godep that we should clean up now that the entire kubelet/rkt package is gone?

Ok so I looked into it, by running:

$ hack/run-in-gopath.sh hack/godep-restore.sh
$ hack/run-in-gopath.sh hack/godep-save.sh

It removed packages github.com/appc/spec, github.com/coreos/go-systemd and go4.org/errorutil.

I don't know how related those are to this PR, from the looks of it, I think they aren't... So I think I'd prefer not to push a Godeps commit in this PR and do that separately. Let me know if you agree with that.

(Also, it turns out my .gitconfig has settings that affect the output of Godeps, in particular the SHA1s in Godeps.json, so my running it produces some spurious changes that would probably best be avoided...)

Cheers!
Filipe

@dashpole

This comment has been minimized.

Contributor

dashpole commented Mar 22, 2018

The first two are related and were imports in the rkt directory, and should be removed. The third doesn't look related, but it is generally a good thing to remove go dependencies that are unused.

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 22, 2018

The first two are related and were imports in the rkt directory, and should be removed. The third doesn't look related, but it is generally a good thing to remove go dependencies that are unused.

Ok, let me adjust my .gitconfig temporarily and re-run that... I'll produce an extra commit to push on top of this one.

Cheers,
Filipe

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 22, 2018

Ok, so I added a second commit to handle Godeps and a third to remove the references from the shell scripts. PTAL.

Cheers!
Filipe

mkdir ${ACI_DIR}
# Download rkt

This comment has been minimized.

@yujuhong

yujuhong Mar 26, 2018

Contributor

@jingxu97 is it ok to remove the rkt code from the gci mounter script?

This comment has been minimized.

@filbranden

This comment has been minimized.

@jingxu97

jingxu97 Mar 28, 2018

Contributor

Yes, I think so.

@yujuhong

Looks good overall. I'm not sure about the GCI mounter bits, and would like defer to storage team (@jingxu97) to confirm.

RemoteContainerRuntime = "remote"
// rkt is deprecated and it won't really work.
// We break execution when it's requested.

This comment has been minimized.

@yujuhong

yujuhong Mar 26, 2018

Contributor

Add a TODO stating when this can be removed

This comment has been minimized.

@filbranden

filbranden Mar 27, 2018

Member

I removed the constant (since it's used in a single place) but I left in place the check that breaks and warns to use rktlet instead. I imagine we can leave that check around, it doesn't really hurt a lot... Right?

filbranden added some commits Mar 20, 2018

Remove rktnetes code
rktnetes is scheduled to be deprecated in 1.10 (#53601). According to
the deprecation policy for beta CLI and flags, we can remove the feature
in 1.11.

Fixes #58721
Update Godeps after removing rkt.
This was done by executing the following two commands:

  $ hack/run-in-gopath.sh hack/godep-save.sh
  $ hack/run-in-gopath.sh hack/godep-restore.sh

Go packages github.com/appc/spec and github.com/coreos/go-systemd were
used by the rkt/ package that is now gone.
@filbranden

This comment has been minimized.

Member

filbranden commented Mar 27, 2018

/retest

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 27, 2018

/test pull-kubernetes-verify

It's saying "Could not resolve host: github.com" 😵

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 28, 2018

/retest

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Mar 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 28, 2018

@dashpole

This comment has been minimized.

Contributor

dashpole commented Mar 28, 2018

/lgtm

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 28, 2018

Now I'm seeing what looks like a stack overflow in RangeAllocator-KubeQPS10-Nodes10... I've seen that before and it looks like it affect other PRs too... Let's keep trying.

/retest

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Mar 28, 2018

FYI, filed #61854 for the ipam test failure.

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 28, 2018

Let's see if we'll bump into #61854 again...
/retest

@filbranden

This comment has been minimized.

Member

filbranden commented Mar 29, 2018

/assign @cblecker
Can you please approve for Godeps/ + vendor/ + hack/ ?

/assign @vishh
Can you please approve for cluster/gce/ ?

Thanks!

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Mar 29, 2018

/assign @dchen1107
for approval

@dchen1107

This comment has been minimized.

Member

dchen1107 commented Mar 29, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, filbranden, yujuhong

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 29, 2018

Automatic merge from submit-queue (batch tested with PRs 61904, 61565, 61401, 61432, 61772). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 399b835 into kubernetes:master Mar 29, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation filbranden 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment