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

fix: kata: workaround passthru net mode when get pod ip #2437

Merged

Conversation

zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Nov 6, 2018

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

kata: workaround passthru net mode when get pod ip

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

cherry-pick

Signed-off-by: Ace-Tang <aceapril@126.com>
@zhuangqh zhuangqh requested a review from Ace-Tang November 6, 2018 07:40
@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

❗ No coverage uploaded for pull request head (fix-kata-annotation@f657a6a). Click here to learn what that means.
The diff coverage is n/a.

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Nov 6, 2018
@@ -608,6 +614,10 @@ func (c *CriManager) PodSandboxStatus(ctx context.Context, r *runtime.PodSandbox
}
}

if v, exist := annotations[passthruKey]; exist && v == "true" {
ip = annotations[passthruIP]
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to check the existence of passthruIP?

Copy link
Collaborator

Choose a reason for hiding this comment

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

passthruIP can be empty string?

@allencloud
Copy link
Collaborator

Do we need to fix cri/v1alpha1/cri.go?

This fixes v1alpha2.

@rudyfly
Copy link
Collaborator

rudyfly commented Nov 6, 2018

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 6, 2018
@zhuangqh
Copy link
Contributor Author

zhuangqh commented Nov 6, 2018

Do we need to fix cri/v1alpha1/cri.go?

This fixes v1alpha2.

v1alpha1 is deprecated. We won't provide the maintenance.

@HusterWan HusterWan merged commit c4c5be8 into AliyunContainerService:master Nov 6, 2018
@@ -74,6 +74,12 @@ const (

// networkNotReadyReason is the reason reported when network is not ready.
networkNotReadyReason = "NetworkPluginNotReady"

// passthruKey to specify whether a interface is passthru to qemu
passthruKey = "io.alibaba.pouch.runv.passthru"
Copy link
Contributor

Choose a reason for hiding this comment

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

io.alibaba.pouch.runv.passthru or io.alibaba.pouch.vm.passthru@Ace-Tang

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, should be io.alibaba.pouch.vm.passthru, could you like help to fix this ? @flyer103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants