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

update gke-networking-api in order to use nodetopology CRD #782

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

update gke-networking-api in order to use nodetopology CRD #782

wants to merge 1 commit into from

Conversation

YifeiZhuang
Copy link

To use nodetopology CRD, upgrade gke-networking-api version.

go get github.com/GoogleCloudPlatform/gke-networking-api@bc15495fd43f01b9d0cc9e244f71d024d81450d4 && ./tools/update_vendor.sh

cc. @basantsa1989 @sawsa307

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 14, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If the repository mantainers determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 14, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @YifeiZhuang. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 14, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: YifeiZhuang
Once this PR has been reviewed and has the lgtm label, please assign bowei for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@sawsa307
Copy link

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 14, 2024
@YifeiZhuang
Copy link
Author

@justinsb or @seans3 could you approve this PR? Thanks a lot.

@YifeiZhuang
Copy link
Author

/assign @basantsa1989
@basantsa1989 could you review and lgtm?

@k8s-ci-robot
Copy link
Contributor

@YifeiZhuang: GitHub didn't allow me to assign the following users: basantsa1989.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @basantsa1989
@basantsa1989 could you review and lgtm?

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-sigs/prow repository.

@basantsa1989
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

@basantsa1989: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-sigs/prow repository.

@YifeiZhuang
Copy link
Author

/assign @bowei

@YifeiZhuang
Copy link
Author

@bowei could you approve the PR?
This is part of the Multi-Subnet Cluster change. Thanks a lot.

bowei
bowei reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

minor comments

vendor/github.com/GoogleCloudPlatform/gke-networking-api/apis/network/v1/annotations.go
// InterfaceStatusAnnotationKey is the key of the annotation which shows information of each interface of a pod.
InterfaceStatusAnnotationKey = "networking.gke.io/interface-status"
// NetworkGatewayIPAnnotationKey is the network annotation key used to hold egress NAT gateway IP.
NetworkGatewayIPAnnotationKey = "networking.gke.io/gateway-ip"
Copy link
Member

Choose a reason for hiding this comment

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

suggest networking.gke.io/nat-gateway-ip unless this is meant to be more generic.

Copy link
Author

Choose a reason for hiding this comment

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

This change was reverted in the original vendor gke-networking-aip repo
So we will update the dependency next time to revert it. (We can not bring in the latest dependency because of bug b/380100355) Hope this is not a blocker, it seems not being used anywhere in CCM yet. @liuyuan10 please confirm it is safe and the path forward in the next CCM release.

vendor/github.com/GoogleCloudPlatform/gke-networking-api/apis/network/v1/annotations.go
// ParseInterfaceStatusAnnotation parses the given annotation to InterfaceStatusAnnotation
func ParseInterfaceStatusAnnotation(annotation string) (InterfaceStatusAnnotation, error) {
ret := &InterfaceStatusAnnotation{}
err := json.Unmarshal([]byte(annotation), ret)
Copy link
Member

Choose a reason for hiding this comment

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

suggest, to avoid partially converted results...

if err := json.Unmarshal(); err != nil { return InterfaceStatusAnnotation{}, err }
return ret, nil

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Original PR. This patterns is in the same file, so needs to be fixed. Tracking in issue

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this has to be fixed in https://github.com/GoogleCloudPlatform/gke-networking-api

@YifeiZhuang
Copy link
Author

@bowei could you review and approve the PR?

@mmamczur
Copy link
Contributor

this is just a dependency update, let's have owners of the dependency update in https://github.com/GoogleCloudPlatform/gke-networking-api, and merge this to unblock development

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2024
@YifeiZhuang
Copy link
Author

Thanks @mmamczur could you merge the PR. I do not have the write access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmamczur mmamczur mmamczur left review comments

@bowei bowei bowei left review comments

@justinsb justinsb Awaiting requested review from justinsb

@seans3 seans3 Awaiting requested review from seans3

Assignees

@bowei bowei

@mmamczur mmamczur

Labels
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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@YifeiZhuang @k8s-ci-robot @sawsa307 @basantsa1989 @mmamczur @bowei