Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

network: Remove grpc AddInterface and RemoveInterface api#457

Merged
sboeuf merged 1 commit intokata-containers:masterfrom
amshinde:remove-unused-network-func
Feb 21, 2019
Merged

network: Remove grpc AddInterface and RemoveInterface api#457
sboeuf merged 1 commit intokata-containers:masterfrom
amshinde:remove-unused-network-func

Conversation

@amshinde
Copy link
Copy Markdown
Member

@amshinde amshinde commented Feb 7, 2019

Taking a look at the network code, I saw that the AddInterface and RemoveInterface APIs are not really used/called by the runtime.
Removing unused code.

Fixes #456

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Feb 7, 2019

/test

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 7, 2019

@amshinde

Since I assume kata-runtime is the only consumer of this API, and since this API has never been used, this won't break anything, even if we're talking about upgrades.
But I'm also wondering if we should allow such thing right now. This should be on a list of deprecated functions that we will remove when releasing the next major (hence 2.0).
@egernst @jodh-intel wdyt?

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Feb 8, 2019

I need to fix this PR by removing unused mock code as well.

But I'm also wondering if we should allow such thing right now. This should be on a list of deprecated functions that we will remove when releasing the next major (hence 2.0).

I dont really think we need to wait for 2.0 for this, as this change really does not break anything. The functions although present in the API, they are not used anywhere.

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Feb 8, 2019

@jon may have input on this (see kata-containers/documentation#196).

@amshinde amshinde force-pushed the remove-unused-network-func branch from d714484 to 6fc1818 Compare February 8, 2019 18:56
@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Feb 8, 2019

/retest

@jcvenegas
Copy link
Copy Markdown
Member

jcvenegas commented Feb 12, 2019

@amshinde ping any update on this.? I like the PR but, still see some jobs failing.

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Feb 12, 2019

I see an unrelated java.io.EOFException for the metrics job, I'll restart that. Similarly, I see unrelated failure for the firecracker one. Is the fircracker CI supposed to pass?
I'll restart both of them nonetheless.

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Feb 12, 2019

@amshinde no the Firecracker CI is being affected by the kernel issue kata-containers/runtime#1203 so I will disable it

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Feb 13, 2019

thanks @GabyCT

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Feb 13, 2019

@amshinde now the CI of firecracker is working as I change it from using Ubuntu to Centos (which is using a kernel that is not broken with vsocks)

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 13, 2019

ooi @GabyCT - is that because the Centos kernel is old (did not get the vsock breaking patch), or is very new and updated (has the patch of a patch that fixes the broken vsocks again) ?
We should keep track of which distro has which kernel version and is broken/fixed...

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Feb 13, 2019

@grahamwhaley the kernel is very old

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 19, 2019

@amshinde @GabyCT @grahamwhaley what's the status on this? Is it okay to move forward and merge this PR without having FC CI passing?

@amshinde As you mentioned, this won't break anything, so let's move forward with it.

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Feb 19, 2019

@sboeuf , I enable the FC with centos and it is working if we want the whole CI running this PR needs to be pushed again

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 19, 2019

@GabyCT I could manually retrigger the FC job, right? Any reason that would prevent me from doing this?

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Feb 19, 2019

@sboeuf , I do not think that is able just to re-trigger the job as it does not find jenkins-ci-ubuntu...

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 19, 2019

Oh sorry I missed the fact that you recently moved FC testing from ubuntu to centos because of the vsock regression in recent ubuntu kernels...
Yes a repush should do it.

@amshinde could you please repush so that it will trigger proper CI?

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 19, 2019

/test
yeah @sboeuf something 'moved' on the Jenkins side, so the link to the old job no longer exists. If one really needed to then one could probably hand-craft a job, but even then it might not update the fc item above, as the name has now probably changed as well. A whole /test, like above, should re-run all current CIs...

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 19, 2019

just ignore the old dead ubuntu fc one, and watch the new shiny centos fc one ;-)

@amshinde
network: Remove grpc AddInterface and RemoveInterface api
ae9ce6e 
These grpc APIs are not really used/called by the runtime.
Remove this to clean the code.

Fixes kata-containers#456

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the remove-unused-network-func branch from 6fc1818 to ae9ce6e Compare February 19, 2019 18:44
@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 19, 2019

/test

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 19, 2019

cool! let's see how this goes and I'll merge if all green :)

@sboeuf
Copy link
Copy Markdown

sboeuf commented Feb 20, 2019

@jodh-intel @grahamwhaley PTAL

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 20, 2019

oh, I'm probably not the person to review grpc proto changes... let's try @gnawux @sameo

@sboeuf sboeuf merged commit dee9d12 into kata-containers:master Feb 21, 2019
@egernst egernst mentioned this pull request Feb 26, 2019
Merged
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Mar 27, 2019
vendor: Update agent vendoring for tracing
952afd8 
Re-vendored the agent to access the new `StartTracing()` and
`StopTracing()` gRPC APIs.

Changes introduced:

    d8813c1 release: Kata Containers 1.6.0
    f64fe3c docs: Don't mention runtime guest kernel CLI for agent tracing
    58fcce0 docs: Simplify trace terminology
    8b34866 release: Kata Containers 1.6.0-rc2
    6bd0f7c tracing: Hide trace messages unless tracing enabled
    00cf907 tracing: Add OpenTracing support
    cbd0aae docs: Explain developer mode
    0b7cae1 docs: Add debug options to README
    5cc1df7 docs: Update README
    4383dbf git: Don't ignore "agent"
    ab3e2c7 api: Remove unused variables
    30d98fe build: Indent ifeq test
    46ec60a build: Remove tabs
    272f273 service: Don't shutdown the VM when agent terminates
    bbe06a4 agent: Properly stop the gRPC server
    30bebf5 agent: increase the number of tries to online vcpus
    d5e64f6 tests: Add test for cheking sysctls
    1f9ac24 cpu: re-arrange code to reduce cyclomatic complexity
    4f249bf sysctl: Add complete sysctl support
    94b601b release: Kata Containers 1.6.0-rc1
    e6006cc CODEOWNERS: add group review trigger for proto file changes
    42e664f mount: add virtio-fs mount driver
    ae9ce6e network: Remove grpc AddInterface and RemoveInterface api
    67b2559 cpuset: update parents with all cpus from guest.
    201c487 cgroup: ignore cpuset if can not be applied.
    00f0cf9 pullapprove: remove it
    44d8478 ci: Add a CODEOWNERS file for github ack checks
    0cb5f22 agent: refine unit test for func GetGuestDetails
    96d7acb agent: add interface memHotplugByProbe for memory hotplug
    6007523 agent: add memory hotplug probe interface check in GetGuestDetails
    b4dae5c release: Kata Containers 1.5.0
    56a779e release: Kata Containers 1.5.0-rc2

Note: this change required removing the `AddInterface()` and `RemoveInterface()` implementations
(removed from the agent on kata-containers/agent#457).

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Apr 15, 2019
vendor: Update agent vendoring for tracing
31a31b7 
Re-vendored the agent to access the new `StartTracing()` and
`StopTracing()` gRPC APIs.

Changes introduced:

    00cf907 tracing: Add OpenTracing support
    cbd0aae docs: Explain developer mode
    0b7cae1 docs: Add debug options to README
    5cc1df7 docs: Update README
    4383dbf git: Don't ignore "agent"
    ab3e2c7 api: Remove unused variables
    30d98fe build: Indent ifeq test
    46ec60a build: Remove tabs
    272f273 service: Don't shutdown the VM when agent terminates
    bbe06a4 agent: Properly stop the gRPC server
    30bebf5 agent: increase the number of tries to online vcpus
    d5e64f6 tests: Add test for cheking sysctls
    1f9ac24 cpu: re-arrange code to reduce cyclomatic complexity
    4f249bf sysctl: Add complete sysctl support
    94b601b release: Kata Containers 1.6.0-rc1
    e6006cc CODEOWNERS: add group review trigger for proto file changes
    42e664f mount: add virtio-fs mount driver
    ae9ce6e network: Remove grpc AddInterface and RemoveInterface api
    67b2559 cpuset: update parents with all cpus from guest.
    201c487 cgroup: ignore cpuset if can not be applied.
    00f0cf9 pullapprove: remove it

Note: this change required removing the `AddInterface()` and `RemoveInterface()` implementations
(removed from the agent on kata-containers/agent#457).

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@caoruidong caoruidong caoruidong approved these changes

@jodh-intel jodh-intel jodh-intel approved these changes

@sboeuf sboeuf sboeuf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@amshinde @sboeuf @jodh-intel @jcvenegas @GabyCT @grahamwhaley @caoruidong