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.

Sign up for GitHub

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

Jump to bottom

mark --network-plugin-dir deprecated for kubelet #46327

Merged
merged 1 commit into from
Jun 19, 2017
Merged

mark --network-plugin-dir deprecated for kubelet #46327

merged 1 commit into from
Jun 19, 2017

Conversation

supereagle
Copy link
Contributor

What this PR does / why we need it:

Which issue this PR fixes : fixes #43967

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 24, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @supereagle. 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 @k8s-bot 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.

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.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 24, 2017
@supereagle
Copy link
Contributor Author

/assign @freehan

@resouer
Copy link
Contributor

resouer commented May 24, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2017
@dashpole
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2017
@mtaufen
Copy link
Contributor

mtaufen commented May 24, 2017

Can you file an issue to track the deprecation of this flag and note the issue number in a comment next to the MarkDeprecated call? See #40229 as an example issue.

@freehan
Copy link
Contributor

freehan commented May 24, 2017

Thanks for the PR.

There are some bootstrap scripts that are still using this flag. Can you also switch them to use cni-bin-dir and cni-conf-dir?

@supereagle supereagle mentioned this pull request May 25, 2017
Closed
@supereagle supereagle force-pushed the mark-network-plugin-dir-deprecated branch from 81afddb to 885fda1 Compare May 25, 2017 04:55
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017
@supereagle
Copy link
Contributor Author

@mtaufen Good practice. Have created the issue #46410 to track this, and added the comment.

@supereagle
Copy link
Contributor Author

@freehan Of course, I will update these bootstrap scripts.

@supereagle
Copy link
Contributor Author

According to the comment:

I am not sure how cni and kubenet share these flags. Below is my interpretation.

  • If --network-plugin=cni, --cni-conf-dir and --cni-bin-dir are used with their individual defaults.
  • If --network-plugin=kubenet is used, --network-plugin-dir is used.

The --network-plugin-dir is used for kubenet network, can this flag really be deprecated or removed?

@freehan
Copy link
Contributor

freehan commented May 25, 2017

@supereagle
kubenet is also compatible with cni-bin-dir.

If I remember correctly, cni-bin-dir preempts network-plugin-dir.

@supereagle supereagle force-pushed the mark-network-plugin-dir-deprecated branch from 885fda1 to 9da584e Compare May 26, 2017 05:29
@supereagle
Copy link
Contributor Author

@freehan You are right, thanks for your clarification. I have updated related bootstrap scripts. PTAL.

As some test case code has been updated, kindly ping @Random-Liu .

supereagle
supereagle commented May 26, 2017
View reviewed changes
hack/make-rules/test-e2e-node.sh
@@ -148,7 +148,7 @@ else

# Do not use any network plugin by default. User could override the flags with
# test_args.
test_args='--kubelet-flags="--network-plugin= --network-plugin-dir=" '$test_args
test_args='--kubelet-flags="--network-plugin= --cni-bin-dir=" '$test_args
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can both --network-plugin= and --cni-bin-dir be set through test_args at the same time? A bug here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is for local testing where no network plugin is used. But it should be able to do it without specifying the network-plugin flag. @Random-Liu confirm?

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 think this has no effect on the tests. If it really has error, I will fix it in another PR, no need to block this PR.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2017
@supereagle
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@dashpole
Copy link
Contributor

/unassign

@supereagle supereagle force-pushed the mark-network-plugin-dir-deprecated branch from b41d0f8 to dc9f0f9 Compare June 1, 2017 14:07
@supereagle
Copy link
Contributor Author

@k8s-bot ok to test

@supereagle
Copy link
Contributor Author

@freehan @Random-Liu Could you help to approve again as this PR has been rebased.

@freehan
Copy link
Contributor

freehan commented Jun 5, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@freehan
Copy link
Contributor

freehan commented Jun 5, 2017

@Random-Liu for /approve

@supereagle
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@supereagle
Copy link
Contributor Author

Kindly ping @Random-Liu @zmerlynn for /approve

@supereagle
Copy link
Contributor Author

Could this be in v1.7? Otherwise we will need to wait one more release to remove this flag. @freehan

@freehan
Copy link
Contributor

freehan commented Jun 9, 2017

@Random-Liu for /approve

@freehan freehan added this to the v1.7 milestone Jun 9, 2017
@marun marun added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 12, 2017
@freehan
Copy link
Contributor

freehan commented Jun 13, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

1 similar comment
@supereagle
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@spiffxp
Copy link
Member

spiffxp commented Jun 16, 2017

/retest

@dchen1107
Copy link
Member

/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, freehan, supereagle

Associated issue: 43967

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 19, 2017 •
edited

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

Test name Commit Details Rerun command
pull-kubernetes-node-e2e dc9f0f9 link @k8s-bot pull-kubernetes-node-e2e test this

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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46327, 47166)

@k8s-github-robot k8s-github-robot merged commit cc645a8 into kubernetes:master Jun 19, 2017
@supereagle
Copy link
Contributor Author

@freehan Will this PR be cherry-picked into v1.7? It has been labeled cherrypick-candidate.

@freehan
Copy link
Contributor

freehan commented Jun 27, 2017

I think it has been cherry-picked

@supereagle
Copy link
Contributor Author

@freehan yup, thanks.

@supereagle supereagle deleted the mark-network-plugin-dir-deprecated branch October 8, 2017 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freehan freehan freehan left review comments

Assignees

@mtaufen mtaufen

@freehan freehan

@Random-Liu Random-Liu

@dchen1107 dchen1107

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. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

kubelet 1.6 looks for CNI binaries in wrong path